Add events command to legal-hold#264
Conversation
…tests, update CHANGELONG
…of archives and archive bytes
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ |
CHANGELOG.md
Outdated
|
|
||
| ### Added | ||
|
|
||
| - `code42 legal-hold events` command: |
There was a problem hiding this comment.
Should the command be called search-events instead?
- It'd be a verb, like the rest of our commands
- It'd mirror the
file-events searchcommand
There was a problem hiding this comment.
Changed to search-events > let me know if this is not what you had in mind
| ), | ||
| help="Filter results by event types", | ||
| ) | ||
| @click.option("--begin", **BEGIN_DATE_DICT) |
There was a problem hiding this comment.
This should use the begin_option defined in code42cli.options
There will be several advantages to doing this.
There was a problem hiding this comment.
Sure, I avoided this option initially because the begin option requires a cursor initialized for state, which I didn't think was necessary. I've added a LegalHoldEvents cursor to the cursor_store.py to make it work, but not sure if that's appropriate.
Also, the begin_option is required, which is should not be.
There was a problem hiding this comment.
I wonder if we should implement the --use-checkpoint option for this command and probably also make a send-to. As I could see customers wanting to be able to automate sending this data into a SIEM.
There was a problem hiding this comment.
Oh! @maddie-vargo Sorry, I did forget about that when I made my initial comments.
I do like the idea from @timabrmsn, supporting checkingpointing would be a nice feature. We can always add it later though
There was a problem hiding this comment.
@unparalleled-js would it be possible to use the begin_option but somehow designate it as optional? I left the original options in there for now.
I left checkpoint'ing off for now.
tests/cmds/test_legal_hold.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def all_events_response(mocker): |
There was a problem hiding this comment.
It is better to have mock generator methods actually be generators and not just lists. I know it probably works fine, but I have seen bugs show up because the tests were doing this. There are some subtle differences.
There was a problem hiding this comment.
Let me know if I'm still missing the mark here.
There was a problem hiding this comment.
This works!
You could add more events to the list so that it yields more than once, that way it is more like what will happen in real life (unless there is often only a single event). Not a big deal though.
tests/cmds/test_legal_hold.py
Outdated
| cli, ["legal-hold", "events", "--event-type", "HoldCreated"], obj=cli_state | ||
| ) | ||
|
|
||
| assert "564564654566" in result.output |
There was a problem hiding this comment.
From the test alone, it is not immediately clear why these values are expected.
Maybe a comment saying // From all_events_response would help
There was a problem hiding this comment.
Defined variables at the top here. I don't love the names
OLDER_LEGAL_HOLD_CREATED_EVENT = "564564654566"
NEWER_LEGAL_HOLD_MEMBERSHIP_EVENT = "74533457745"
There was a problem hiding this comment.
The names are fine..
but alternatively they could be called
_CREATE_EVENT_ID and _MEMBERSHIP_EVENT_ID
Reasons:
1.) they are already in a legal hold module so don't need the prefix,
2.) they can be internal since nothing should be importing or using them outside of this test module, and
3.) ending with ID makes it more apparent that it's not a JSON object
There was a problem hiding this comment.
I took your suggestions. Thanks!
|
|
||
| assert "Matter ID,Name,Description,Creator,Creation Date" not in result.output | ||
|
|
||
|
|
There was a problem hiding this comment.
Could we get test(s) for begin_date / end_date validation?
It could be to just assert the values were passed into py42 correctly.
There was a problem hiding this comment.
I'll work on this. I had tried this initially, but had trouble getting a test to recognize the date inputs in the runner.
There was a problem hiding this comment.
I added a test titled test_search_events_is_called_with_expected_begin_timestamp
…as opposed to list, better handling for longer outputs
| @matter_id_option | ||
| @matter_id_option( | ||
| True, | ||
| "Identification number of the legal hold matter the custodian will be added to.", |
There was a problem hiding this comment.
@annie-payseur When you have chance, could you help review these option help texts? I will tag you in spots.
| @matter_id_option | ||
| @matter_id_option( | ||
| True, | ||
| "Identification number of the legal hold matter the custodian will be removed from.", |
src/code42cli/cmds/legal_hold.py
Outdated
| @format_option | ||
| @sdk_options() | ||
| def search_events(state, matter_id, event_type, begin, end, format): | ||
| """Report on legal hold events.""" |
There was a problem hiding this comment.
@annie-payseur This is the command's help text: Report on legal hold events.
There was a problem hiding this comment.
I suggest changing to Tools for getting legal hold event data. (which is more consistent with the help text for security-data)
tests/cmds/test_legal_hold.py
Outdated
| ] | ||
| } | ||
| """ | ||
| EVENTS_RESPONSE = """ |
There was a problem hiding this comment.
Something seems off about this response.... It is not valid JSON. Should these objects be under a list with key: legalHoldEvents
There was a problem hiding this comment.
Or else I believe line 290 will create a response where this a str type (I think).
There was a problem hiding this comment.
I don't think the fixture that uses this is ever used, if that is the, case can this be removed?
There was a problem hiding this comment.
Yes, that is no longer needed, since I moved to a generator format for the mocker. I forgot to take it out on my last push along with it's corresponding fixture.
tests/cmds/test_legal_hold.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def events_response(mocker): |
There was a problem hiding this comment.
Is this fixture used anywhere? I am not seeing.
Perhaps this and EVENTS_RESPONSE can be removed?
There was a problem hiding this comment.
Yes, this is no longer needed. See my comment on the item above.
|
Does anything else need to happen on this PR, or can it be merged and closed? |
Was just waiting for a bugfix release to happen (just did). I can merge this now, but do you mind fixing the CHANGELOG one last time? Just need to add your changes to a new Unreleased section (above the version section that was released today). After that, I will merge this in and get it officially tested and released ASAP. |
4 participants
This PR addresses Phase II of #176 to allow users to list legal hold administrative events. This command includes options to filter the results based on a specific legal hold uid, a beginning timestamp, an end timestamp, and a list of event types.
Testing Procedure
PR Checklist
Did you remember to do the below?